Build hook spec #955
Build hook spec #955
Conversation
PR HealthBreaking changes ✔️Details
Changelog Entry ✔️Details
Changes to files need to be accounted for in their respective changelogs. Coverage ✔️Details
This check for test coverage is informational (issues shown here will not fail the PR). License Headers ✔️Details
All source files should start with a license header. Unrelated files missing license headers
Package publish validation ✔️Details
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
Co-authored-by: Moritz <mosum@google.com>
Co-authored-by: Moritz <mosum@google.com>
Co-authored-by: Moritz <mosum@google.com>
|
|
||
| Currently, all such assets are dynamic libraries. | ||
|
|
||
| An asset must have an `assetId`. Dart code that uses an asset, references the asset using the `assetId`. |
There was a problem hiding this comment.
Here and in the other items: You probably mean the metadata must contain? Also, what does using mean? I assume just referencing, as an asset is a file.
There was a problem hiding this comment.
Right, we have conflated the Asset class that is metadata, and the file. This probs requires a full rewrite.
Since we also have a "metadata" field in the build input and build output, we shoudl probably refrain from calling everything that is not the file metadata. Maybe we should call the file the "asset file".
|
|
||
| An asset must have an `assetId`. Dart code that uses an asset, references the asset using the `assetId`. | ||
|
|
||
| An asset must have a target os and target architecture. When accessing the asset at runtime, the asset for the current os and architecture is accessed. |
There was a problem hiding this comment.
I thought there is not necessarily a target architecture in dry run mode?
There was a problem hiding this comment.
Not as input, but there is for the output. We should probably fix that in v2.
| An asset must specify a path type for dynamic linking. | ||
|
|
||
| * `absolute` paths are absolute paths in the output dir (see below). | ||
| * `system` paths are expected to resolve on the target machine PATH. In this case the asset is not a file but only metadata. |
There was a problem hiding this comment.
In this case the asset is not a file but only metadata. well that is not really agreeing with what an asset is. I would update the definition above.
|
|
||
| * `absolute` paths are absolute paths in the output dir (see below). | ||
| * `system` paths are expected to resolve on the target machine PATH. In this case the asset is not a file but only metadata. | ||
| * `process` "paths" have no path, symbols are resolved in the current process. In this case the asset is not a file but only metadata. |
There was a problem hiding this comment.
"paths" have no path - What does this mean? What are paths? What are symbols - assets are files, right?
There was a problem hiding this comment.
Yeah so code assets are already much more involved than "a file". Code assets can be metadata only. And some variants of that metadata do not require any path.
path_type is probably not the right name.
Maybe it should be dynamic_library_type. But the executable and process one don't even have a dynamic library.
So then it should be dynamic_linking_type. This also sounds weird, because the linking always is the same in a way.
Maybe it should be dynamic_resolution_type. That gets closer. But the problem here is that we're also abstracting over the resolution partially. Dart and Flutter repackage dynamic libraries. So in this protocol for absolute paths, it's the absolute path at the point of this protocol. But for system paths, process, and executable the metadata gets passed in unmodified.
So then it should maybe be something like:
- asset_id: ...
dynamic_resolution:
type: bundled_dylib
# no path here, because that will be decided by Flutter/Dart when making the bundle.
path: ... # Not nested in dynamic_resolution, it's the path for the protocol.
- asset_id: ...
dynamic_resolution:
type: system
path: ... # Nested in dynamic_resolution, it's the metadata to be passed into linking.
- asset_id: ...
dynamic_resolution:
type: process
- asset_id: ...
dynamic_resolution:
type: executablewdyt?
| * Write assets into `out_dir` from `build_config.yaml`. | ||
| * If `dry_run: true` in `build_config.yaml`, then this may be skipped. | ||
| * Filename are unrelated to `assetId`. | ||
| * Arbitrary file names are fine, `build_output.yaml` will map `assetId` to files. |
There was a problem hiding this comment.
The file names of the assets don't really matter. It could be file1.foo, file2.foo, file3.foo. Because the only way assets are accessed from Dart code is through their assetId.
However, we do want to have to files written to disk rather than having a buffer of bytes for files, because quite often the files are produces by an external Process call or for data assets already in your package.
| * If `dry_run: true` in `build_config.yaml`, then this may be skipped. | ||
| * Filename are unrelated to `assetId`. | ||
| * Arbitrary file names are fine, `build_output.yaml` will map `assetId` to files. | ||
| * MUST avoid file name `build_output.yaml`. |
There was a problem hiding this comment.
If you write your asset to out_dir/build_output.yaml it's going to fail to write build_output.yaml
Maybe in v2 we should consider writing the assets somewhere else than the build output.
| * MUST avoid file name `build_output.yaml`. | ||
| * Write `build_output.yaml` into `out_dir` (from `build_config.yaml`). | ||
| * This maps `assetId` to assets previous written in `out_dir`. | ||
| * There may be multiple assets for a given `assetId` depending on |
There was a problem hiding this comment.
You might want to add something about the uniqueness of an assetId - this was unclear in an earlier review comment as well.
|
@dcharkes I pushed some suggestions, if you want to take a look. |
|
Okay, I think this is in a good enough shape. I'll merge this and we can do more edits later. 👍 |
Revisions updated by `dart tools/rev_sdk_deps.dart`. crypto (https://github.com/dart-lang/crypto/compare/c954015..f059196): f059196 2024-02-08 Kevin Moore Test dart2wasm (dart-archive/crypto#162) dartdoc (https://github.com/dart-lang/dartdoc/compare/457c34e..f152c01): f152c013 2024-02-08 Sam Rawlins Tidy spacing in templates (dart-lang/dartdoc#3652) d0c53b9f 2024-02-07 Sam Rawlins Change all late final Iterable fields to Lists (dart-lang/dartdoc#3649) 8c9c7790 2024-02-06 Sam Rawlins Remove unnecessary ExtensionTarget mixin (dart-lang/dartdoc#3648) d2f90c5a 2024-02-06 Sam Rawlins Fix the 'serve testing-package' task to generate docs (dart-lang/dartdoc#3647) ecosystem (https://github.com/dart-lang/ecosystem/compare/7d8ec47..3e4f286): 3e4f286 2024-02-08 Moritz Add ignore flag for `publish.yaml` (dart-lang/ecosystem#237) lints (https://github.com/dart-lang/lints/compare/90a61e4..ead7708): ead7708 2024-02-08 Devon Carew add library_annotations; remove package_prefixed_library_names (dart-lang/lints#179) native (https://github.com/dart-lang/native/compare/876f9a1..cb9bd7e): cb9bd7ef 2024-02-09 Daco Harkes Build hook spec (dart-lang/native#955) sse (https://github.com/dart-lang/sse/compare/e483b14..af7d8d0): af7d8d0 2024-02-09 Ilya Yanok Make `SseConnection` extend `StreamChannelMixin<String>` (dart-archive/sse#102) Change-Id: I2e85dbfae00eea8de7b6f7efce1993bf00e243f0 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/352046 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Commit-Queue: Devon Carew <devoncarew@google.com> Auto-Submit: Devon Carew <devoncarew@google.com>
There are multiple differences between the `native_assets.yaml` that is embedded in the kernel file and the `build_output.yaml -> assets`. * Based on the discussions on #955 and #946, it is clear that the `path` for assets should be in `Asset`, not in `AssetPath` for the file-path the asset has after the `build.dart` run. When the embedders (Flutter/Dart) embed the native assets mapping then the `path`s start representing the path on the system where the Dart/Flutter app is running. This should be embedded in the path-type there. * The kernel info does not contain link mode (currently), as static linking is not supported. It's not clear that if we support static linking whether any information should be embedded in the kernel info at all. * The native_assets.yaml for the kernel file is laid out for easy lookup at runtime (keyed on Target). * We want to change the `Asset`s to output OS and Architecture instead of Target. Therefore we should not share the data structure between `Asset`s and `KernelAsset`s (new name for the entries that are embedded via native_assets.yaml in a kernel file.) This means that `dartdev` and `flutter_tools` will have to start converting `Asset`s to `KernelAsset`s when making the `native_assets.yaml` file. Therefore this is a breaking change and will have to be rolled into Dart and flutter/flutter manually.
To be followed up by a v2.md